👋 Hello jimmyw, we appreciate your contribution to this project!

Generated by 🚫 dangerJS against 7af818b

Hi @jimmyw,

Thanks for raising the PR!

Is there a specific reason why you want to change the way of data sending to atomic (header+payload) style? I am afraid the logic change is too big for the current UART driver. Also, the atomic (header+payload) way would take more space in the ring buffer, since every data chunk needs a header now.

It is reasonable to introduce a new API to have the timeout parameter to allow the bytes writing to return when getting blocked. However, this can be achieved in a relatively simple way: by just changing the portMAX_DELAY in uart_tx_all function to the timeout parameter, right?

@songruo

Hi, thanks for your reply.

Yes, there is a very good reason for doing header + data in one atomic operation, and that is if you manage to fit a header in the buffer, and and not data (Due to timeout) the uart will get of and think that the next header you add is the data, as it expects a data after header.

I also think the code is cleaner, and more understandable this way, and with less overhead as we do less locking.

It wont take much more buffer space, every write is currently an header + data today, even if you only write 1 byte to the uart. Its only when you write very big chunks in the buffer, it will be split up in 4 parts just to improve buffer stale lock handling. you dont want to fill up the buffer with a single piece of data, as that will dma block the full messagebuffer, and a less prioritized process wont be able to feed it in time, causing a buffer under-run.

I added now so if the buffer have less space free, then whats we like to add to the buffer, it will split that into two, having the first write non blocking, and the second part will obviously block until timeout. This is an optimization, that we can remove.

I am all for alternative suggestions on how to solve this.

Just to make clear what i am trying to do with this PR, i give you an example (not completely sure this will happen in practice, but we hit a similar scenario):

lets say we run PPP over a modem with flow control, LWIP stack wants to write 1500bytes to the uart, but the modem is busy with other stuff, not having a timeout on uart write will make the complete LWIP stack hang until the modem says so. And when it says, the tcp timeout on what we tried to send to the modem has passed already, so that would eventally be discarded anyway.

Adding a timeout, makes LWIP notice that package was not send, and it can go on with other stuff, retrying later or dropping the package. Today with a blocking operation, it will sitt there wait 4 ever.

@jimmyw You are right, header + data in one atomic operation is necessary if we want to add the timeout option. The PR generally LGTM, I could sync it to our internal repo, but I don't guarantee it will be accepted due to the large UART TX logic change involved. We need to do some internal discussion and heavy testings to ensure no breaking change will be introduced.

Thank you @songruo, i also did not expect adding a timeout would need this complex change. But the flip coin is that.

1. Somewhat simplified code.
2. Less locking and task switching overhead just using one entry per write should improve.
3. Improved buffer utilization spitting writes that does not fit.
4. Less stalls on big writes, as we dont need to wait for the full write to fit buffer.
5. The tx_lock can also be removed now, if you only send from a single task. This brings the locks down from 3 to 1 writing a small buf.
6. A timeout on send.

Think these are good reasons you could bring with you.